20260512-wc_init_state#10472
Conversation
douzzer
requested review from
julek-wolfssl,
rizlik,
wolfSSL-Bot and
wolfSSL-Fenrir-bot
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #10472
Scan targets checked: wolfcrypt-bugs, wolfcrypt-src
No new issues found in the changed files. ✅
douzzer
force-pushed
the
20260512-wc_init_state
branch
2 times, most recently
from
17b7825 to
74aea5b
Compare
|
|
retest this please |
julek-wolfssl
left a comment
julek-wolfssl
left a comment
There was a problem hiding this comment.
This solution is quite complex. Can't we use the same approach of two vars (atomic + volatile) from wolfSSL_Init? The main improvement I can see in the relation between wolfSSL_Init and wolfSSL_Cleanup is that wolfSSL_Cleanup should not do anything if inits_count_mutex_valid is not 1.
|
This solution uses a single 32 bit atomic. That's the simplest implementation supported by the hardware. We're only doing this because code review uncovered races in the previous implementation. And the refactored implementation only looks complex because I added a bunch of error-checking. |
douzzer
force-pushed
the
20260512-wc_init_state
branch
from
74aea5b to
df7d3da
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #10472
Scan targets checked: wolfcrypt-bugs, wolfcrypt-src
No new issues found in the changed files. ✅
douzzer
force-pushed
the
20260512-wc_init_state
branch
2 times, most recently
from
2339d63 to
f1c0935
Compare
julek-wolfssl
left a comment
julek-wolfssl
left a comment
There was a problem hiding this comment.
All these new assumptions and limitations should be documented in doc/. At least the change to a busy loop, BAD_STATE blocking indefinitely, and compiler differences.
* wc_local_InitUp() * wc_local_InitUpDone() * wc_local_InitDown() * wc_local_InitDownDone() * wc_init_state_t * WC_DECLARE_INIT_STATE() * WC_INIT_STATE_* * union wc_init_state_bitfields * WC_INIT_STATE_RAISE_BAD_STATE() * WC_ATOMIC_INT_ARG and WC_ATOMIC_UINT_ARG, pivoting on WC_16BIT_CPU, used to assure operands to atomic operators are 32 bits, and that wc_init_state_t is 32 bits, even on 16 bit targets like Arduino. fix&refactor thread safety mechanisms in wolfCrypt_Init() and wolfCrypt_Cleanup(), and fix a few preexisting error-handling flubs in wolfCrypt_Init().
douzzer
force-pushed
the
20260512-wc_init_state
branch
from
f1c0935 to
a36ada8
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #10472
Scan targets checked: none
Failed targets: wolfcrypt-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
There was a problem hiding this comment.
Pull request overview
This PR refactors wolfCrypt_Init() / wolfCrypt_Cleanup() to use an explicit atomic state machine for initialization depth tracking, aiming to eliminate previously unmitigated multi-threaded races and ensure consistent initialization/cleanup behavior under contention.
Changes:
- Introduces
wc_local_InitUp/Downandwc_init_state_t-based state machine to coordinate init/cleanup transitions. - Adjusts atomic API argument/return types via
WC_ATOMIC_{INT,UINT}_ARG(notably forWC_16BIT_CPU) and propagates those through atomic function signatures. - Adds
WC_INIT_ERROR_WHEN_CONTENDEDas a configurable behavior knob for contention handling.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
wolfssl/wolfcrypt/wc_port.h |
Adds init-state types/macros and widens atomic argument typedefs for 16-bit targets. |
wolfcrypt/src/wc_port.c |
Implements the init/cleanup state machine and rewires wolfCrypt_Init/Cleanup to use it; updates atomic backends’ signatures. |
.wolfssl_known_macro_extras |
Registers WC_INIT_ERROR_WHEN_CONTENDED for tooling/macro awareness. |
Comments suppressed due to low confidence (2)
wolfcrypt/src/wc_port.c:1718
- wolfSSL_Atomic_Int_CompareExchange() assigns the observed value back to expected_i using an (int) cast, but expected_i is now WC_ATOMIC_INT_ARG. This can truncate when WC_ATOMIC_INT_ARG is wider than int.
int wolfSSL_Atomic_Int_CompareExchange(wolfSSL_Atomic_Int* c,
WC_ATOMIC_INT_ARG *expected_i,
WC_ATOMIC_INT_ARG new_i)
{
u_int exp = (u_int) *expected_i;
int ret = atomic_fcmpset_int(c, &exp, new_i);
*expected_i = (int)exp;
return ret;
wolfcrypt/src/wc_port.c:1728
- wolfSSL_Atomic_Uint_CompareExchange() assigns the observed value back to expected_i using an (unsigned int) cast, but expected_i is now WC_ATOMIC_UINT_ARG. This can truncate when WC_ATOMIC_UINT_ARG is wider than unsigned int.
int wolfSSL_Atomic_Uint_CompareExchange(
wolfSSL_Atomic_Uint* c, WC_ATOMIC_UINT_ARG *expected_i,
WC_ATOMIC_UINT_ARG new_i)
{
u_int exp = (u_int)*expected_i;
int ret = atomic_fcmpset_int(c, &exp, new_i);
*expected_i = (unsigned int)exp;
return ret;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
wolfcrypt/src/wc_port.c: fix&refactor thread safety mechanisms inwolfCrypt_Init()andwolfCrypt_Cleanup().This PR fixes unmitigated races in the incumbent code whereby three or more threads racing each other are subject to UB, with one or more of the threads returning success with the library not actually initialized.
The new algorithm is airtight, always succeeding unless API abuse or memory corruption.
tested with